Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SYCL][CMake][MSVC] Fix link.exe /OPT detection #16811

Merged
merged 1 commit into from
Jan 29, 2025
Merged

[SYCL][CMake][MSVC] Fix link.exe /OPT detection #16811

merged 1 commit into from
Jan 29, 2025

Conversation

Maetveis
Copy link
Contributor

CMake's check_linker_flag does no split flags by spaces, so the current call passes the single option "/OPT:REF LINKER:/OPT:ICF" with a space in it to link.exe. (The first LINKER: prefix is parsed). This was also broken before ede906c ([CMake][MSVC] Wrap more Linker flags for ICX (#16284)), where it would pass "/OPT:REF /OPT:ICF" as a single option.

This results in the check failing and so the build does not ever enable these flags, even though they would be supported if the check was correct.

Use comma as the separator as supported by the LINKER: syntax to fix it.

@Maetveis
[SYCL][CMake][MSVC] Fix link.exe /OPT detection
c7d577f 
CMake's check_linker_flag does no split flags by spaces, so the current
call passes the single option `"/OPT:REF LINKER:/OPT:ICF"`
with a space in it to link.exe. (The first `LINKER:` prefix is parsed).
This was also broken before ede906c
([CMake][MSVC] Wrap more Linker flags for ICX (intel#16284)), where it would
pass `"/OPT:REF /OPT:ICF"` as a single option.

This results in the check failing and so the build does not ever enable
these flags, even though they would be supported if the check was correct.

Use comma as the separator as supported by the `LINKER:` syntax to fix it.
@Maetveis Maetveis requested a review from a team as a code owner January 28, 2025 12:09
@Maetveis
Copy link
Contributor Author

On CMake not supporting multiple flags in check_linker_flags

Failing Example:

cmake_minimum_required(VERSION 3.23)

project(ASD LANGUAGES CXX)

include(CheckLinkerFlag)

check_linker_flag(CXX "/OPT:REF /OPT:ICF" LINKER_SUPPORTS_OPTS)

Configured with MSVC:

> cmake -S . -B build
-- Building for: Visual Studio 17 2022
-- Selecting Windows SDK version 10.0.22000.0 to target Windows 10.0.20348.
-- The CXX compiler identification is MSVC 19.40.33811.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.40.33807/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test LINKER_SUPPORTS_OPTS
-- Performing Test LINKER_SUPPORTS_OPTS - Failed
-- Configuring done (3.2s)
-- Generating done (0.0s)
-- Build files have been written to: asd/build

Relevant part from build/CMakeFiles/CMakeConfigureLog.yaml

-
    kind: "try_compile-v1"
    backtrace:
      - "Modules/Internal/CheckSourceCompiles.cmake:108 (try_compile)"
      - "Modules/Internal/CheckLinkerFlag.cmake:37 (cmake_check_source_compiles)"
      - "Modules/CheckLinkerFlag.cmake:44 (cmake_check_linker_flag)"
      - "CMakeLists.txt:7 (check_linker_flag)"
    checks:
      - "Performing Test LINKER_SUPPORTS_OPTS"
    directories:
      source: "asd/build/CMakeFiles/CMakeScratch/TryCompile-e9pd8p"
      binary: "asd/build/CMakeFiles/CMakeScratch/TryCompile-e9pd8p"
    cmakeVariables:
      CMAKE_CXX_FLAGS: "/DWIN32 /D_WINDOWS /EHsc"
      CMAKE_CXX_FLAGS_DEBUG: "/Zi /Ob0 /Od /RTC1"
      CMAKE_EXE_LINKER_FLAGS: "/machine:x64"
    buildResult:
      variable: "LINKER_SUPPORTS_OPTS"
      cached: true
      stdout: |
        Change Dir: 'asd/build/CMakeFiles/CMakeScratch/TryCompile-e9pd8p'
        
        Run Build Command(s): "C:/Program Files/Microsoft Visual Studio/2022/Professional/MSBuild/Current/Bin/amd64/MSBuild.exe" cmTC_27211.vcxproj /p:Configuration=Debug /p:Platform=x64 /p:VisualStudioVersion=17.0 /v:n
        MSBuild version 17.10.4+10fbfbf2e for .NET Framework
        Build started 1/28/2025 3:42:53 AM.
        
        Project "asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\cmTC_27211.vcxproj" on node 1 (default targets).
        PrepareForBuild:
          Creating directory "cmTC_27211.dir\\Debug\\".
          Structured output is enabled. The formatting of compiler diagnostics will reflect the error hierarchy. See https://aka.ms/cpp/structured-output for more details.
          Creating directory "asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\Debug\\".
          Creating directory "cmTC_27211.dir\\Debug\\cmTC_27211.tlog\\".
        InitializeBuildStatus:
          Creating "cmTC_27211.dir\\Debug\\cmTC_27211.tlog\\unsuccessfulbuild" because "AlwaysCreate" was specified.
          Touching "cmTC_27211.dir\\Debug\\cmTC_27211.tlog\\unsuccessfulbuild".
        ClCompile:
          C:\\Program Files\\Microsoft Visual Studio\\2022\\Professional\\VC\\Tools\\MSVC\\14.40.33807\\bin\\HostX64\\x64\\CL.exe /c /Zi /W1 /WX- /diagnostics:column /Od /Ob0 /D _MBCS /D WIN32 /D _WINDOWS /D LINKER_SUPPORTS_OPTS /D "CMAKE_INTDIR=\\"Debug\\"" /EHsc /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"cmTC_27211.dir\\Debug\\\\" /Fd"cmTC_27211.dir\\Debug\\vc143.pdb" /external:W1 /Gd /TP /errorReport:queue "asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\src.cxx"
          Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64
          Copyright (C) Microsoft Corporation.  All rights reserved.
          cl /c /Zi /W1 /WX- /diagnostics:column /Od /Ob0 /D _MBCS /D WIN32 /D _WINDOWS /D LINKER_SUPPORTS_OPTS /D "CMAKE_INTDIR=\\"Debug\\"" /EHsc /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"cmTC_27211.dir\\Debug\\\\" /Fd"cmTC_27211.dir\\Debug\\vc143.pdb" /external:W1 /Gd /TP /errorReport:queue "asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\src.cxx"
          src.cxx
        Link:
          C:\\Program Files\\Microsoft Visual Studio\\2022\\Professional\\VC\\Tools\\MSVC\\14.40.33807\\bin\\HostX64\\x64\\link.exe /ERRORREPORT:QUEUE /OUT:"asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\Debug\\cmTC_27211.exe" /INCREMENTAL /ILK:"cmTC_27211.dir\\Debug\\cmTC_27211.ilk" /NOLOGO kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /manifest:embed /DEBUG /PDB:"asd/build/CMakeFiles/CMakeScratch/TryCompile-e9pd8p/Debug/cmTC_27211.pdb" /SUBSYSTEM:CONSOLE /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"asd/build/CMakeFiles/CMakeScratch/TryCompile-e9pd8p/Debug/cmTC_27211.lib" /MACHINE:X64  /machine:x64 "/OPT:REF /OPT:ICF" cmTC_27211.dir\\Debug\\src.obj
        LINK : fatal error LNK1117: syntax error in option 'OPT:REF /OPT:ICF' [asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\cmTC_27211.vcxproj]
        Done Building Project "asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\cmTC_27211.vcxproj" (default targets) -- FAILED.
        
        Build FAILED.
        
        "asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\cmTC_27211.vcxproj" (default target) (1) ->
        (Link target) -> 
          LINK : fatal error LNK1117: syntax error in option 'OPT:REF /OPT:ICF' [asd\\build\\CMakeFiles\\CMakeScratch\\TryCompile-e9pd8p\\cmTC_27211.vcxproj]

Working Example:

cmake_minimum_required(VERSION 3.23)

project(ASD LANGUAGES CXX)

include(CheckLinkerFlag)

check_linker_flag(CXX "LINKER:/OPT:REF,/OPT:ICF" LINKER_SUPPORTS_OPTS)
> cmake -S . -B build
-- Building for: Visual Studio 17 2022
-- Selecting Windows SDK version 10.0.22000.0 to target Windows 10.0.20348.
-- The CXX compiler identification is MSVC 19.40.33811.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.40.33807/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test LINKER_SUPPORTS_OPTS
-- Performing Test LINKER_SUPPORTS_OPTS - Success
-- Configuring done (5.4s)
-- Generating done (0.0s)
-- Build files have been written to: asd/build

steffenlarsen
steffenlarsen approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Looks like CI is having some Windows build issues. I've seen that elsewhere, but I am not sure what the problem is.

@Maetveis
Copy link
Contributor Author

Maetveis commented Jan 28, 2025 •
edited
Loading

Would appreciate some help here, tar is crashing when trying to pack the build artifacts. Its likely that the size of the install tree changed (probably decreased) because of the linker optimizations being enabled now, but I have no idea why that would lead to a crash in tar.

This happened even after I restarted the job.

> Run tar -czf llvm_sycl.tar.gz -C install .
D:\github\_work\_temp\44bf5c8d-be24-4ec7-a841-fa3d68a93fb9.sh: line 1:   528 Segmentation fault      (core dumped) tar -czf llvm_sycl.tar.gz -C install .
Error: Process completed with exit code 139.

@steffenlarsen
Copy link
Contributor

It doesn't seem to be related. I see it on other PRs as well, e.g. #16819. I will try and retrigger CI.

@Maetveis
Copy link
Contributor Author

It doesn't seem to be related. I see it on other PRs as well, e.g. #16819. I will try and retrigger CI.

Thanks, it passed now.

@intel/llvm-gatekeepers, can you please merge? :)

@steffenlarsen steffenlarsen merged commit 0b3f4e7 into intel:sycl Jan 29, 2025
17 checks passed
@Maetveis Maetveis deleted the fix_check_linker_flag branch January 29, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenlarsen steffenlarsen steffenlarsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Maetveis @steffenlarsen